Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type-cast enums to int before comparison #7

Merged
merged 1 commit into from
Nov 13, 2021

Conversation

giordano
Copy link
Contributor

@giordano giordano commented Nov 10, 2021

In glibc 2.33, ST_* and MS_* are enums, not macros: https://sourceware.org/git/?p=glibc.git;a=blob;f=bits/statvfs.h;h=998deca502aa5a67ae937a05e1c077b6c647b10c;hb=9826b03b747b841f5fc6de2054bf1ef3f5c4bdf3. This means they need to be type-casted to int (or any other integer type) in order to compare them without getting a warning (which are fatal errors with -Werror). Fixes #6.

@parke parke changed the base branch from master to devel November 13, 2021 00:35
@parke parke merged commit a9cdb42 into parke:devel Nov 13, 2021
@giordano giordano deleted the mg/enums-if-eq branch November 13, 2021 00:37
@parke
Copy link
Owner

parke commented Nov 13, 2021

I have create a devel branch of Lxroot and merged your pull request into it. I'm actually going to tweak the fix before merging(?) it back into master.

This is the first time I've used branches on Github, so if there is a better way I could have effected this change, please let me know.

Thanks again for trying Lxroot, reporting the bug, and submitting your pull request.

@matthewpersico
Copy link

No, that's a perfectly reasonable way to handle it. Just make sure that you put that person's handle in the commit comments somewhere so "credit" is maintained. That merge will eventually have your name on it but you never know when some entity will challenge your code for being a non original work; a good audit trail wouldn't be a bad idea.

@matthewpersico
Copy link

Although the contributor's name will probably be on the commit before you tweak. Just make sure you rebase and merge; if you squash and merge with the GitHub ui, be sure to leave the contributor's name in the text.

@parke
Copy link
Owner

parke commented Nov 15, 2021

@matthewpersico - Thanks for mentioning issues related to original work. After contemplation, I have decided that I will avoid pulling (what I consider to be) trivial fixes or contributions into Lxroot.

@StefanKarpinski
Copy link

StefanKarpinski commented Nov 15, 2021

It's standard practice—best practice, even—to compile things with -Werror, which converts warnings into compiler errors, so that warnings that may reflect undefined behavior in C, as it does here. Without this patch the project does not compile with that flag, which makes the project seem broken.

I'm not entirely sure that I follow @matthewpersico's concerns about attribution: if you merge this into a devel branch and later merge into or rebase onto another branch, then git retains the attribution (authorship) of those commits. Attribution is only lost if you squash changes, but that's not typical.

@giordano
Copy link
Contributor Author

After contemplation, I have decided that I will avoid pulling (what I consider to be) trivial fixes or contributions into Lxroot.

I'm having a hard time understanding the rationale of this conclusion. You simply replaced casting with int

lxroot/lxroot.cpp

Lines 731 to 732 in a9cdb42

#define if_equal( a, b ) ( (int)a == (int)b ? b : 0 )
#define if_not_equal( a, b ) ( ( (int)a != (int)b ) && ( n & a ) ? b : 0 )

with casting with flags_t:

lxroot/lxroot.cpp

Lines 731 to 733 in 9aa7440

#define c(a) ( (flags_t) (a) )
#define if_equal( a, b ) ( c(a) == c(b) ? b : 0 )
#define if_not_equal( a, b ) ( ( c(a) != c(b) ) && ( n & a ) ? b : 0 )

because you deemed my contribution to be trivial and so you don't want my name to appear in your history?

The Free Software Foundation, which is somewhat overly cautious about taking on contributions from "third parties" as you call them, doesn't even consider trivial contributions to be legally significant and doesn't require copyright assignment for them. You seem to be rejecting exactly this kind of contributions. If this is your policy you'd be better off making it clear in the README file.

@parke
Copy link
Owner

parke commented Nov 15, 2021

Regarding casting to flags_t (which is a typedef of mine, not an external type), this choice was inspired by man 2 statfs. Specifically:

   The  __fsword_t  type  used  for various fields in the statfs structure
   definition is a glibc internal type, not intended for public use.  This
   leaves  the  programmer  in a bit of a conundrum when trying to copy or
   compare these fields to  local  variables  in  a  program.   Using  un‐
   signed int for such variables suffices on most systems.

And also that, according to man 2 mount, the type of mountflags is unsigned long. (flags_t is a my typedef of unsigned long.)

Also, I believe it is best practice with macros to enclose macro parameters with parenthesis, as macros do text substitution, which can lead to order-of-operation confusion unless parenthesis are used. Granted, I was not doing this with if_equal and if_not_equal, but now that I am nesting macro c inside of those two, I want to clean up usage of macro parameters.

Regarding credit for contributions, I'm open to hearing what would make you happy.

If I added a thanks.txt file, and documented your contribution there (along with other contributions past and future, would that be satisfactory?

I guess I'm hoping to avoid:

  1. Pull requests with fewer than 10 lines where I subsequently rewrite or remove all the contributed lines.
  2. Credit/attribution comments in the source code itself.

I am certainly willing to discuss further.

I agree with your suggestion that, whatever contribution policy I end up adopting, I should describe it in the README file.

@StefanKarpinski
Copy link

StefanKarpinski commented Nov 15, 2021

I guess I'm hoping to avoid:

  1. Pull requests with fewer than 10 lines where I subsequently rewrite or remove all the contributed lines.

Are you hoping to avoid having to do that work? Given that there is an issue here, even if this exact patch doesn't end up being the solution, it seems that something needs to be fixed. Or are you just worried about who gets credit for the work?

  1. Credit/attribution comments in the source code itself.

There's certainly no legal obligation to provide credit/attribution of any contribution in the source code and doing so would be unusual for an open source project. It is typically considered sufficient in open source projects for the git history to record who contributed what (and even that isn't really needed). I don't think @giordano expects any sort of attribution or credit in the source—please contradict me if I'm wrong, @giordano—he just wants the compilation warning to be fixed in the upstream project.

@parke
Copy link
Owner

parke commented Nov 15, 2021

@StefanKarpinski

Are you hoping to avoid having to do that work? Given that there is an issue here, even if this exact patch doesn't end up being the solution, it seems that something needs to be fixed. Or are you just worried about who gets credit for the work?

I believe the issue is already fixed in Lxroot's (new) main branch.

main is a new branch, and is currently the (new) default branch of Lxroot.

I'm worried about a third-party claiming ownership of Lxroot. For example, in the United States, an employer owns an employee's work product. If an employee submits a pull-request, and I accept it that pull request, then the employer can claim that Lxroot is now a derived work of the pull request, and that therefore the employer has an ownership interest in Lxroot's source code. Potentially, the employer could object to Lxroot being distributed under the GPLv3 license.

This may not be a realistic concern in the context of the two pull requests I have already received.

Potentially, someday, I might want to re-license Lxroot under a different license. It is much easier for me to re-license Lxroot if I am the sole author.

The two pull requests I have received to date do not, in my opinion, contribute sufficient value to Lxroot to be worth the cost of contaminating my sole-authorship of Lxroot. Maybe someday I will receive a pull-request that merits diluting my claim of sole-authorship. But I have not yet, in my opinion, received such a pull request.

@StefanKarpinski
Copy link

Ah, it seems that the intention then is not for lxroot to be open source in the usual sense, which generally includes allowing people to contribute improvements to the project. Have you considered a CLA? That would allow people to contribute code while retaining your ability to relicense the code.

@StefanKarpinski
Copy link

StefanKarpinski commented Nov 16, 2021

Some comments about specific legal concerns. Regarding your desire to retain the right to relicense your code, it seems like you could use a CLA to accomplish that, which would also somewhat mitigate your concern about someone contributing code that their employer later claims to own. Also note that such a situation would not give the company a right to claim ownership of the entire project, it would at most give them the right to ask you to stop using the code contributed by their employee. If you were to revert those changes when notified of the issue, then you'd have made a good faith effort to comply with copyright to the best of your knowledge, and you'd be in the clear legally. The company could sue their employee but they wouldn't have a case against you or the lxroot project.

Regarding the size and substance of patches, I believe you've got it backwards: the small, insignificant patches—like this one—that you're rejecting are exactly the ones that are safe to accept, because they're too trivial to be copyrightable. The larger and more significant contributions that you're willing to accept are the the ones that would be copyrightable.

As a higher level comment, a bunch of us who work on Julia saw your recent talk about lxroot at PackagingCon and were quite excited about it. Great presentation! We do some similar stuff with Linux namespaces for lightweight, nestable containerization. But lxroot seems much more featureful, so it seemed like something we should look into using. @giordano immediately went to try to compile it and hit this issue; being a good open source citizen, he submitted this patch. However, the ensuing conversation here and unwillingness to accept improvements is a bit surprising and definitely leaves me—and I suspect other Julia folks–dismayed and reluctant to use lxroot. Even if lxroot is better and more featureful, we're better off using our own code that's properly open source than a nominally open source project that doesn't accept fixes or other contributions. I'm definitely not trying to tell you how you should run your project—it is yours and you should do what you prefer—I'm just being direct about the impression made by this thread. We will probably not use lxroot, which is unfortunate because it's quite cool.

@parke
Copy link
Owner

parke commented Nov 16, 2021

Ah, it seems that the intention then is not for lxroot to be open source in the usual sense, which generally includes allowing people to contribute improvements to the project. Have you considered a CLA? That would allow people to contribute code while retaining your ability to relicense the code.

As it relates to community contributions, my long term plans and expectations are not clear at present, not even to myself.

I have thought about a CLA, but only in a general, abstract sense. If someone wanted to contribute a specific addition to Lxroot, I would be happy to discuss that addition and whether or not it aligns with my vision for Lxroot.

My vision for Lxroot is that Lxroot is a minimal, small, compact program. At multiple levels: user interface, compiled binary size, and (slightly less importantly) lines of source code.

At present, I add features to Lxroot when I want to use them myself. Currently, Lxroot has all the features I want and need for my use cases. I am aware of some missing features (for example, Wayland support) that I expect I would be willing to add as soon as some user tells me they want to use Lxroot with Wayland.

@parke
Copy link
Owner

parke commented Nov 16, 2021

Thank you for the continued discussion of these issues.

As a higher level comment, a bunch of us who work on Julia saw your recent talk about lxroot at PackagingCon and were quite excited about it. Great presentation! We do some similar stuff with Linux namespaces for lightweight, nestable containerization. But lxroot seems much more featureful, so it seemed like something we should look into using. @giordano immediately went to try to compile it and hit this issue; being a good open source citizen, he submitted this patch. However, the ensuing conversation here and unwillingness to accept improvements is a bit surprising and definitely leaves me—and I suspect other Julia folks–dismayed and reluctant to use lxroot.

So I see four issues here:

  1. My current hesitancy to accept small "non-copyrightable" patches.

Let's imagine I decide to accept "non-copyrightable" patches.

  1. My complete rewriting of small patches after I accept them (such as in the case of this pull request).

Is this a problem for you?

  1. My desire to retain the ability to relicense Lxroot.

Let's imagine I am willing to relinquish this to be able to accept substantial contributions.

(Aside: I only started thinking about relicensing Lxroot because someone told me they could not use GPL software in their project. And I don't think I'm willing to go to a MIT or BSD license, but I might be willing to sell a license.)

Even if lxroot is better and more featureful, we're better off using our own code that's properly open source than a nominally open source project that doesn't accept fixes or other contributions. [emphasis added]

  1. My vision for Lxroot and how that will affect the types of features I am willing to integrate and support.

I see a big difference between fixes versus other contributions.

I've discussed fixes in 1 & 2 above.

Regarding other contributions, there would be two types.

(a) Contributions that align with my vision for Lxroot as a small, compact tool.

(b) Contributions that necessitate making Lxroot larger and more complicated than I want it to be.

I can imagine a future in which some users of Lxroot will want to add features that I consider to be "too big" or "too complicated". I cannot say exactly where than line is. It would depend on the specific addition.

If you have specific features you can see you want to add to Lxroot, I might be able to tell you whether or not they align with my vision for Lxroot.

I'm definitely not trying to tell you how you should run your project—it is yours and you should do what you prefer—I'm just being direct about the impression made by this thread. We will probably not use lxroot, which is unfortunate because it's quite cool.

I'm willing to keep an open mind about your desires and requirements. Nothing is set in stone. I've never led an open source project before, so all these issues are new to me, and I appreciate your willingness to discuss.

So, I guess to summarize:

How do you feel about me accepting and then re-writing "non-copyrightable" fixes (2, above)?

How do you feel about my preference to keep Lxroot a small, compact tool (4b, above)?

@StefanKarpinski
Copy link

StefanKarpinski commented Nov 16, 2021

So I see four issues here:

  1. My current hesitancy to accept small "non-copyrightable" patches.

Let's imagine I decide to accept "non-copyrightable" patches.

  1. My complete rewriting of small patches after I accept them (such as in the case of this pull request).

Is this a problem for you?

It's absolutely no problem if someone submitting a fix propels you to fix the same problem in a different way—so long as the problem gets fixed! I've been on both sides of that kind of exchange many times.

Btw, it's not entirely clear that looking at someone's patch and then rewriting it really absolves you of copyright claims. Some people go to extreme lengths to avoid having even looked at an implementation with a viral license (like the GPL) in case their rewrite might be deemed to be derived. I personally think that's overkill and unnecessary, but I'm not a lawyer. In this case, the patch is so small as to be clearly not copyright-worthy, so it doesn't matter. And as I said, as long as the issue gets fixed, that's the only thing I would care about.

  1. My desire to retain the ability to relicense Lxroot.

Let's imagine I am willing to relinquish this to be able to accept substantial contributions.

(Aside: I only started thinking about relicensing Lxroot because someone told me they could not use GPL software in their project. And I don't think I'm willing to go to a MIT or BSD license, but I might be willing to sell a license.)

If you're unsure, maybe just do a CLA so that you can at least accept patches without hesitation. That way you have the ability to change license later if you want to.

  1. My vision for Lxroot and how that will affect the types of features I am willing to integrate and support.

I see a big difference between fixes versus other contributions.

I've discussed fixes in 1 & 2 above.

Regarding other contributions, there would be two types.

(a) Contributions that align with my vision for Lxroot as a small, compact tool.

(b) Contributions that necessitate making Lxroot larger and more complicated than I want it to be.

I can imagine a future in which some users of Lxroot will want to add features that I consider to be "too big" or "too complicated". I cannot say exactly where than line is. It would depend on the specific addition.

If you have specific features you can see you want to add to Lxroot, I might be able to tell you whether or not they align with my vision for Lxroot.

It seems all well and good to me for you to stick with your vision of a small, compact lxroot. We don't have any features at all in mind—the project just didn't compile. It's fine for an open source maintainer to say no to things they don't feel fit their vision–essential even for a healthy project. Believe me, saying "no" is something I have to do a lot of. The issue here is not that anyone wants you to agree to some blank check policy where you'll accept any and all changes that anyone proposes. That would be a disaster. The issue is that that not accepting fixes is a red flag for an open source project.

The setup here is: Mosè tries to compile lxroot, gets a compiler error, fixes the error and submits a patch. What one would expect is one of: (a) "thanks!" and a merge (b) "thanks, but there's a better way to fix this; here's my alternative fix" or (c) "here's why we can't do this, let's discuss other approaches". This interaction is unsettling because there's a lot of legal stuff and implications about maybe not accepting changes from people at all. Or maybe accepting some changes provided they're big enough. Or maybe only if you personally completely rewrite them. All of that suggests that making other fixes in the future will be a problem, let alone suggesting other improvements.

So perhaps you could think about whether you're willing to accept fixes or proposals for improvements or not (subject to you considering them fitting of your lxroot vision, of course), and figuring out if you want to do a CLA or what, and let us know.

@parke
Copy link
Owner

parke commented Nov 16, 2021

Btw, it's not entirely clear that looking at someone's patch and then rewriting it really absolves you of copyright claims.

Yes, I agree. I could have been clearer about my motivations for rewriting. My primary motivations for rewriting a patch would probably be:

  1. improving (IMO) readability
  2. improving (IMO) maintainability
  3. conformance with my personal coding style (which seems to be a moving target as my style seems to continually evolve)
  4. correctness

If you're unsure, maybe just do a CLA so that you can at least accept patches without hesitation. That way you have the ability to change license later if you want to.

So perhaps you could think about whether you're willing to accept fixes or proposals for improvements or not (subject to you considering them fitting of your lxroot vision, of course), and figuring out if you want to do a CLA or what, and let us know.

Are there any CLAs or template CLAs that you are familiar and comfortable with, that I might be able to use or adapt?

@parke
Copy link
Owner

parke commented Nov 16, 2021

The issue is that that not accepting fixes is a red flag for an open source project. The setup here is: Mosè tries to compile lxroot, gets a compiler error, fixes the error and submits a patch. What one would expect is one of: (a) "thanks!" and a merge (b) "thanks, but there's a better way to fix this; here's my alternative fix"

How would I submit such an alternative fix? What buttons would I click in GitHub, or what commands do I type on the command line?

@giordano
Copy link
Contributor Author

You can either leave a comment here as you're doing now, or go to the "files changed" tab, click on a line or select multiple lines to leave inline comments. You can also suggest changes by clicking on the +- button there. You can read github documentation about this: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews

@StefanKarpinski
Copy link

I would usually make a completely new pull request with the alternative change and then link to it from a comment saying "Here's an alternative fix: [url to alternative fix]".

Unfortunately (or fortunately, depending on your point of view), I don't really have any experience with using or signing CLAs—Julia and all the other projects I'm a maintainer on are simply MIT licensed with no CLA. I've done a little googling and https://contributoragreements.org/ca-cla-chooser/ looks reasonably legit—it's from the Free Software Foundation Europe, which is a legitimate organization affiliated with the Free Software Foundation. There's also https://cla-assistant.io which looks like a handy way to allow people to sign CLAs on GitHub.

@StefanKarpinski
Copy link

Note that if you use an MIT license, you can kind of forget about all of this since if you want to relicense under a different license at some point in the future, you're free to do so even without a CLA—you can just make a proprietary fork and give it whatever other license you want. Of course, so is anyone else. My rule of thumb is that if something seems like a product, then you may want to GPL it, but if it's infrastructure, then something like MIT is a better choice. Here's what I wrote about it a while ago:

In terms of choosing a license for an open source project, I think you should choose the most permissive license that will sufficiently encourage people not to make proprietary forks. The GPL was designed on the premise that it would be necessary to legally force people to contribute changes back, but the success of so many projects with MIT and BSD licenses shows this often isn't necessary. I think the choice depends on how "infrastructurey" or "producty" the project is. For projects like libraries, compilers or programming languages, it turns out that proprietary forks don't happen because of simple economics. Why would anyone pay for a forked proprietary version of a library when the original open source project is available? And if no one wants to directly pay for a forked version of some library, why would any company bother maintaining a fork when they can just contribute their improvements back and let the open source community maintain them for free?

With a self-contained product like Light Table [an IDE that was hot when I wrote this], on the other hand, there's a very real danger that some company could come along, fork the code base, make a bunch of improvements and start selling their version. Using a viral open source license like the GPL makes this illegal, ensuring that only the original author can sell forks of the product.

tl;dr – GPLv3 is a great choice of license for Light Table because it is a product, but it's overkill for most open source projects which provide much lower level functionality.

@parke
Copy link
Owner

parke commented Nov 17, 2021

@StefanKarpinski

Thanks for the links about CLAs and the MIT license.

At present, Linux is licensed under the GPL, and Lxroot only works with Linux. Consequently, for better or for worse, I am not inspired to use a more permissive license than the GPL at present.

Regarding CLAs: I looked at Wikipedia and saw that Golang has a CLA. I would be willing adapt the Golang CLA to accept contributions for Lxroot.

https://cla.developers.google.com/about/google-individual
https://cla.developers.google.com/about/google-corporate

I don't feel a CLA is necessary for this (#7) pull request.

I am considering setting up three branches for Lxroot:

  • master
  • testing
  • contrib (formerly named devel)

master and contrib already exist. I have not yet created the testing branch, but hope to soon.

I will pull third-party contributions into contrib. Once they've been edited to conform with my coding standards, I will push them to testing. With some frequency, I will merge testing into master.

This pull request (#7) has already been accepted into contrib.

In the future, I will try to comment on pull requests before accepting them into contrib, rather than just pulling them in and rewriting them to comply with my coding standards.

If anyone has suggestions for improvements or alternatives to this workflow, I would be happy to hear them.

@StefanKarpinski
Copy link

StefanKarpinski commented Nov 17, 2021

Regarding CLAs: I looked at Wikipedia and saw that Golang has a CLA. I would be willing adapt the Golang CLA to accept contributions for Lxroot.

This is getting very meta, but I'm not sure what the license of that CLA is, so I'm not certain if you're allowed to adapt it. It is almost certainly copyright of Google and doesn't say anything about reuse.

If anyone has suggestions for improvements or alternatives to this workflow, I would be happy to hear them.

If that works for you, it sounds reasonable. It's your process, you'll have to try it out and see how it goes and probably make some modifications based on experience. But starting out with something that makes sense to you is always a good way to go.

@parke
Copy link
Owner

parke commented Nov 18, 2021

This is getting very meta, but I'm not sure what the license of that CLA is, so I'm not certain if you're allowed to adapt it.

A fair point, thanks.

@parke
Copy link
Owner

parke commented Nov 18, 2021

Please let me know if you have any comments or suggestions on the below pull request.

#9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compilation fails with Glibc 2.33
4 participants